Skip to content

fix(workflows): assert HEAD on per-agent branch before worktree commits (#2924)#2941

Merged
trek-e merged 4 commits intomainfrom
fix/2924-worktree-head-attachment
May 1, 2026
Merged

fix(workflows): assert HEAD on per-agent branch before worktree commits (#2924)#2941
trek-e merged 4 commits intomainfrom
fix/2924-worktree-head-attachment

Conversation

@trek-e
Copy link
Copy Markdown
Collaborator

@trek-e trek-e commented May 1, 2026

Fix PR


Linked Issue

Fixes #2924


What was broken

The worktree-mode setup in execute-phase.md/execute-plan.md/quick.md could leave HEAD attached to a protected branch (master/main) inside a freshly-spawned agent worktree, causing the agent's git commit calls to advance the protected branch instead of the agent branch. The "recovery" path then ran git update-ref refs/heads/master <fork_sha> to silently force-rewind the protected ref — destroying concurrent commits in multi-active scenarios (parallel agents, or a user committing while an agent runs). Worktree-mode commits also defaulted to --no-verify, contradicting project CLAUDE.md guidance and silently skipping pre-commit hooks.

What this fix does

  1. HEAD-attachment assertion before any reset/checkout<worktree_branch_check> in execute-phase.md and quick.md now runs git symbolic-ref --quiet HEAD and git rev-parse --abbrev-ref HEAD before any git reset --hard. If HEAD is detached or on a protected ref (main|master|develop|trunk|release/*), the workflow HALTs with a clear blocker. The pointer in execute-plan.md (Pattern A) mirrors the new ordering.
  2. Per-commit HEAD assertionagents/gsd-executor.md <task_commit_protocol> gains a mandatory step 0 that re-asserts the same invariant before every git commit in worktree mode (HEAD attachment can drift after git checkout <sha> or similar).
  3. Destructive recovery removed and forbidden<destructive_git_prohibition> in agents/gsd-executor.md now lists git update-ref refs/heads/<protected> as an absolute prohibition with explicit guidance: surface the blocker, never self-heal. The new regression test enforces this across every workflow markdown file.
  4. --no-verify no longer a worktree-mode default — Removed from <parallel_execution> in execute-phase.md, <precommit_failure_handling> in execute-plan.md, the pre-dispatch plan commit in quick.md, and the parallel-agents callout in references/git-integration.md. Pre-commit hooks now run normally; opt-out is via workflow.worktree_skip_hooks=true. The post-wave hook-validation step in execute-phase.md is gated on the same flag.

Root cause

In the original worktree-mode setup, git reset --hard <master_sha> ran inside the worktree before HEAD's symbolic-ref had been pointed at the agent branch. HEAD therefore stayed attached to refs/heads/master, and any subsequent git commit advanced master. The "recovery" was a destructive workaround for the setup bug, not a real fix — it papered over the symptom by force-rewinding the protected ref, which works in single-active scenarios by luck and silently destroys data in multi-active ones. The --no-verify default existed for cargo-lock contention in parallel commits but was applied unconditionally, so it stripped hook coverage from every worktree-mode commit regardless of whether contention was actually a concern.

Testing

How I verified the fix

npm test                                                    # 6245/6245 pass
node --test tests/bug-2924-worktree-head-attachment.test.cjs  # new regression test, all green
node --test tests/bug-2015-worktree-base-branch.test.cjs      # existing reset-base test still passes
node --test tests/workflow-size-budget.test.cjs               # execute-phase.md within XL budget (1696/1700)

The new regression test parses files structurally — extracts named XML-like blocks, tokenizes shell statements inside fenced code blocks (handling $(...) substitution and VAR=$(...) assignments), then asserts statement-index ordering. Per repo policy: no regex / .includes() against raw file content for load-bearing assertions.

Regression test added?

Platforms tested

  • macOS
  • Windows (including backslash path handling)
  • Linux
  • N/A (not platform-specific)

Runtimes tested

  • Claude Code
  • Gemini CLI
  • OpenCode
  • Other: ___
  • N/A (not runtime-specific)

Checklist

  • Issue linked above with Fixes #NNNPR will be auto-closed if missing
  • Linked issue has the confirmed-bug label
  • Fix is scoped to the reported bug — no unrelated changes included
  • Regression test added (or explained why not)
  • All existing tests pass (npm test)
  • CHANGELOG.md updated if this is a user-facing fix
  • No unnecessary dependencies added

Files changed

  • agents/gsd-executor.md — pre-commit HEAD assertion (step 0) in <task_commit_protocol>; added git update-ref refs/heads/<protected> to <destructive_git_prohibition> (cites bug(execute-plan): worktree HEAD attaches to master; agent commits land on master; "recovery" force-rewinds master via git update-ref #2924).
  • get-shit-done/workflows/execute-phase.md<worktree_branch_check> asserts git symbolic-ref HEAD before any git reset --hard; <parallel_execution> no longer mandates --no-verify; post-wave hook validation gated on workflow.worktree_skip_hooks.
  • get-shit-done/workflows/execute-plan.md — Pattern A pointer rewritten to require Step 1 HEAD assertion before Step 2 base check; <precommit_failure_handling> no longer mandates --no-verify for parallel executors.
  • get-shit-done/workflows/quick.md<worktree_branch_check> adds the same Step 1 HEAD assertion; pre-dispatch plan commit gates --no-verify behind workflow.worktree_skip_hooks.
  • get-shit-done/references/git-integration.md — parallel-agents callout flipped to "hooks run by default."
  • tests/bug-2924-worktree-head-attachment.test.cjs — new structural regression test.

Judgment calls

  • The orchestrator does not know the per-agent branch name at template-emit time (Claude Code's isolation="worktree" chooses worktree-agent-<id> internally), so the assertion rejects protected refs rather than matching a specific expected name. This is strictly stronger than the snippet in the issue.
  • Kept --no-verify as a config-gated opt-in (workflow.worktree_skip_hooks=true) rather than removing the capability entirely — the cargo-lock contention rationale for parallel commits is real for some projects, but it is now opt-in instead of the default.
  • execute-phase.md has an XL line budget (≤ 1700). Condensed the new block twice to land at 1696 lines (was 1698 before).

Breaking changes

Behaviorally, projects that relied on the previous --no-verify worktree-mode default now have their pre-commit hooks run on every executor commit. Affected projects can restore the previous behavior with workflow.worktree_skip_hooks=true in .planning/config.json. No file-format or CLI-flag changes.

Summary by CodeRabbit

  • New Features

    • Added workflow.worktree_skip_hooks option to opt out of running pre-commit hooks during worktree commits.
  • Bug Fixes

    • Enforced mandatory HEAD attachment and per-agent branch namespace; rejects detached HEADs and protected branches.
    • Prohibited destructive updates to protected refs (including forced updates/pushes); disallowed automated self-recovery.
    • Pre-dispatch commits are idempotent and fail-fast on commit errors.
  • Documentation

    • Updated guidance to reflect branch safety, hook defaults, and opt-out behavior.
  • Tests

    • Added regression tests validating safety gates and hook rules.

Worktree-mode setup could leave HEAD attached to a protected branch (master),
causing agent commits to land there. The previous response was a destructive
self-recovery via 'git update-ref refs/heads/master <sha>', which silently
rewinds the protected branch and destroys concurrent commits in multi-active
scenarios (parallel agents, user committing while agent runs).

- Reorder <worktree_branch_check> in execute-phase.md and quick.md to assert
  HEAD via 'git symbolic-ref' BEFORE any 'git reset --hard'. HALT with a
  blocker if HEAD is on main/master/develop/trunk/release/* or detached.
- Add a per-commit HEAD assertion (step 0) to gsd-executor.md
  <task_commit_protocol>; HEAD attachment can drift after 'git checkout <sha>'.
- Forbid 'git update-ref refs/heads/<protected>' in
  <destructive_git_prohibition>; surface the blocker rather than self-heal.
- Remove '--no-verify' as the worktree-mode default in execute-phase.md,
  execute-plan.md, quick.md, and references/git-integration.md. Hooks now
  run on every executor commit; opt out only via workflow.worktree_skip_hooks.
- Add regression test that parses the worktree_branch_check blocks structurally
  and asserts the symbolic-ref check precedes the reset --hard, no workflow
  performs update-ref on a protected ref, and --no-verify is no longer the
  default in any parallel-execution prompt.
@trek-e trek-e requested a review from glittercowboy as a code owner May 1, 2026 04:01
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0469be5b-4b3a-4966-9ca5-64141207a3f4

📥 Commits

Reviewing files that changed from the base of the PR and between 03bbb5c and cd5c6ae.

📒 Files selected for processing (1)
  • tests/bug-2924-worktree-head-attachment.test.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/bug-2924-worktree-head-attachment.test.cjs

📝 Walkthrough

Walkthrough

Adds mandatory worktree-only HEAD/branch assertions that fatal-exit on detached or protected refs and require worktree-agent-* branches; forbids destructive self-recovery (e.g., git update-ref refs/heads/<protected>) and forced pushes by agents; runs pre-commit hooks by default (opt-out via workflow.worktree_skip_hooks); adds regression tests.

Changes

Cohort / File(s) Summary
Safety Prohibitions & Docs
agents/gsd-executor.md, get-shit-done/references/git-integration.md, docs/CONFIGURATION.md
Add explicit prohibitions on git update-ref refs/heads/<protected> and forced pushes from agents; document new workflow.worktree_skip_hooks flag and remove default guidance to bypass pre-commit hooks.
Worktree Safety & Executors
get-shit-done/workflows/execute-phase.md, get-shit-done/workflows/execute-plan.md, get-shit-done/workflows/quick.md
Introduce mandatory HEAD validation (fail if detached, on protected refs, or not worktree-agent-*) before any reset/commit; forbid automated self-recovery that rewinds protected refs; change commit behavior to run hooks by default and gate --no-verify behind workflow.worktree_skip_hooks.
Config Schema
get-shit-done/bin/lib/config-schema.cjs, sdk/src/query/config-schema.ts
Add workflow.worktree_skip_hooks to the exact-match VALID_CONFIG_KEYS allowlist.
Regression Tests
tests/bug-2924-worktree-head-attachment.test.cjs
Add Node.js regression test that parses workflow/agent markdown to assert HEAD-attachment checks precede resets, forbid unguarded git update-ref, and validate hook-bypass gating and commit behavior.

Sequence Diagram(s)

(Skipped.)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • #2924 — Directly addresses the reported worktree HEAD-attachment and destructive git update-ref recovery described in the issue.
  • gsd-build/get-shit-done#2686 — Matches the HEAD attachment, branch-namespace, and git update-ref prohibition objectives.

Possibly related PRs

  • gsd-build/get-shit-done#2718 — Modifies the config-schema allowlist; overlaps with this PR's config-key changes.
  • gsd-build/get-shit-done#2561 — Edits VALID_CONFIG_KEYS and docs; related to adding workflow config keys.
  • gsd-build/get-shit-done#2479 — Changes to the same config-schema module; closely related at the file level.

Suggested reviewers

  • glittercowboy

Poem

🐰 I hopped into the worktree, whiskers keen and bright,
Found HEAD tethered wrong — no more will that take flight.
Guards now stop the tumble, no secret rewinds at night,
Hooks hum on each commit, agents behave polite. 🥕

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'assert HEAD on per-agent branch before worktree commits (#2924)' accurately and concisely summarizes the main bug fix—adding a HEAD attachment assertion before worktree commits—and references the specific issue. It is clear and specific without noise.
Description check ✅ Passed The description uses the correct fix template, includes all required sections (What was broken, What this fix does, Root cause, Testing, Checklist), provides detailed context, linked issue with Fixes #2924, regression test justification, test results, and comprehensive file-by-file explanation of changes.
Linked Issues check ✅ Passed The PR fully addresses all coding requirements from issue #2924: adds HEAD-attachment assertions before resets, forbids destructive git update-ref on protected refs, removes --no-verify default from worktree commits, implements per-commit re-assertions, and adds comprehensive regression tests.
Out of Scope Changes check ✅ Passed All changes directly address the linked issue #2924: HEAD assertions in workflows, destructive operation prohibition, --no-verify removal, config flag support, and regression tests. Documentation updates (CONFIGURATION.md, config schemas) support the feature flag changes. No unrelated changes present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/2924-worktree-head-attachment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
tests/bug-2924-worktree-head-attachment.test.cjs (1)

186-189: ⚡ Quick win

Prefer structured assertions over raw string checks

These checks still rely on raw .includes()/substring sentence scans. Since this suite already parses blocks/statements, assert on parsed structure for these cases too to reduce brittleness and align with repository test policy.

Based on learnings: “follow the CONTRIBUTING.md ‘no-source-grep’ testing standard… avoid regex/string-grep approaches… assert on parsed/structured representation.”

Also applies to: 288-291, 390-405

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/bug-2924-worktree-head-attachment.test.cjs` around lines 186 - 189, The
tests currently assert by checking raw strings with block.includes('update-ref')
in the worktree_branch_check test and similar checks at the other noted ranges;
instead locate the parsed representation produced by the test harness (e.g., the
parsed block/statement AST or the array like blocks/statements returned by the
parser used in this file) and assert the presence and properties of a specific
node (for example assert that a node of type 'command' or 'update-ref' exists,
or that a recovery action node has name 'update-ref' and expected flags) rather
than doing a substring search; update the assertions in worktree_branch_check
and the other occurrences (lines ~288-291 and ~390-405) to inspect the parsed
node types/fields (the same parsed structure used elsewhere in this suite) and
assert on those structured fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@agents/gsd-executor.md`:
- Around line 361-369: The current pre-commit check (using HEAD_REF and
ACTUAL_BRANCH) only denies detached or protected refs but still permits
arbitrary branches (e.g., feature/*); change it to require a positive per-agent
branch allow-list by asserting ACTUAL_BRANCH matches an explicit agent namespace
pattern (for example a regex like '^agent/|^agents/|^claude-agent/'); if
ACTUAL_BRANCH does not match the allow-list, print the same FATAL message and
exit non-zero — keep the existing DETACHED/protected checks and do NOT attempt
any self-recovery via git update-ref.

In `@get-shit-done/workflows/execute-phase.md`:
- Around line 658-660: The workflow uses the config key
workflow.worktree_skip_hooks but that key is not registered in the SDK config
schema; open sdk/src/query/config-schema.ts and add
"workflow.worktree_skip_hooks" to the exported set/array/object of valid config
keys (look for symbols like VALID_CONFIG_KEYS, validKeys, or registerConfigKey)
and include a default/description entry if the schema structure requires it so
the key is recognized by gsdk query config-get and the worktree skip-hooks
opt-out functions correctly.
- Around line 661-663: The script currently runs "git diff --cached --quiet ||
git stash" to stash unstaged changes but never restores them; change this to
create a named stash (e.g., git stash push -u -m "pre-hook-$(date +%s)") and
record whether a stash was created, then after "git hook run pre-commit" (and
after the post-wave hook sequence) conditionally restore it with "git stash pop"
(or "git stash pop --index" to restore index) and handle pop failures gracefully
(print a warning and exit non-zero if conflicts occur); reference the existing
commands "git diff --cached --quiet", "git stash" and "git hook run pre-commit"
when implementing the stash/create-flag and the subsequent "git stash pop"
restore logic.

In `@get-shit-done/workflows/quick.md`:
- Around line 642-647: The pre-dispatch git commit currently swallows failures
via "|| true"; remove that so real commit errors surface. Update the SKIP_HOOKS
branch and the normal branch where git commit is invoked (the lines using
SKIP_HOOKS, git commit --no-verify and git commit) to drop "|| true" and instead
check the commit exit status and abort the script on failure (print a clear
error referencing QUICK_DIR/${quick_id}-PLAN.md, quick_id and DESCRIPTION) by
exiting non-zero so the dispatcher cannot proceed without the PLAN.md committed.

In `@tests/bug-2924-worktree-head-attachment.test.cjs`:
- Around line 171-177: Extend the test's required protected-ref set by adding
'trunk' and 'release/*' to the required array so the loop that iterates over
required (for (const name of required)) asserts those refs too; ensure the regex
construction (new RegExp(`\\b${name}\\b`)) accounts for the literal asterisk in
'release/*' (escape or match the slash and asterisk) so the
assert.ok(re.test(scripts), `worktree_branch_check must reference '${name}' in
its protected-branch list`) correctly verifies the presence of 'trunk' and the
'release/*' protected-ref.
- Around line 359-363: The current workflowFiles collection only reads top-level
files under path.join(REPO_ROOT, 'get-shit-done', 'workflows') and misses nested
.md files; change the discovery used by workflowFiles to recursively walk the
workflows directory (or use a glob like **/*.md) so it collects all Markdown
files in subdirectories as well, then map those results to absolute paths as
before (reference workflowFiles and the path.join(REPO_ROOT, 'get-shit-done',
'workflows') call to locate where to replace the non-recursive readdirSync).

---

Nitpick comments:
In `@tests/bug-2924-worktree-head-attachment.test.cjs`:
- Around line 186-189: The tests currently assert by checking raw strings with
block.includes('update-ref') in the worktree_branch_check test and similar
checks at the other noted ranges; instead locate the parsed representation
produced by the test harness (e.g., the parsed block/statement AST or the array
like blocks/statements returned by the parser used in this file) and assert the
presence and properties of a specific node (for example assert that a node of
type 'command' or 'update-ref' exists, or that a recovery action node has name
'update-ref' and expected flags) rather than doing a substring search; update
the assertions in worktree_branch_check and the other occurrences (lines
~288-291 and ~390-405) to inspect the parsed node types/fields (the same parsed
structure used elsewhere in this suite) and assert on those structured fields.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d23dc6d-0662-4ad5-8c7f-a03314de691c

📥 Commits

Reviewing files that changed from the base of the PR and between 7e9477b and ec57850.

📒 Files selected for processing (6)
  • agents/gsd-executor.md
  • get-shit-done/references/git-integration.md
  • get-shit-done/workflows/execute-phase.md
  • get-shit-done/workflows/execute-plan.md
  • get-shit-done/workflows/quick.md
  • tests/bug-2924-worktree-head-attachment.test.cjs

Comment thread agents/gsd-executor.md
Comment thread get-shit-done/workflows/execute-phase.md Outdated
Comment thread get-shit-done/workflows/execute-phase.md Outdated
Comment thread get-shit-done/workflows/quick.md Outdated
Comment thread tests/bug-2924-worktree-head-attachment.test.cjs Outdated
Comment thread tests/bug-2924-worktree-head-attachment.test.cjs
- Add positive worktree-agent-* allow-list to <task_commit_protocol> step 0
  in gsd-executor.md and to <worktree_branch_check> in execute-phase.md and
  quick.md. The deny-list (main|master|develop|trunk|release/*) silently
  allowed feature/* and other arbitrary branches outside the agent namespace.
- Register workflow.worktree_skip_hooks in both config schemas
  (sdk/src/query/config-schema.ts and get-shit-done/bin/lib/config-schema.cjs)
  and document it in docs/CONFIGURATION.md so config-set accepts it.
- Fix stash lifecycle in execute-phase.md post-wave hook validation: stash
  under a named ref and pop after the hook run; warn on pop failure.
- Pre-dispatch PLAN.md commit in quick.md: gate on git diff --cached --quiet
  for idempotency and exit 1 with a clear error on commit failure (both the
  --no-verify and the normal branches) — no more swallowing real errors.
- Test fixes (tests/bug-2924-worktree-head-attachment.test.cjs):
  - Parse the protected-branch alternation structurally and require
    main, master, develop, trunk, release/.* (release/* was previously
    skipped by the \\b...\\b regex).
  - Use fs.readdirSync(dir, { recursive: true }) so workflows in nested
    subdirectories are also asserted against the update-ref ban.
  - Add allow-list assertions for execute-phase.md, quick.md, and
    gsd-executor.md to lock in the new positive namespace check.
@github-actions github-actions Bot added size/XL and removed size/XL labels May 1, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/bug-2924-worktree-head-attachment.test.cjs (1)

202-210: 🏗️ Heavy lift

Prefer structured assertions over raw text matching in this suite.

This file still uses source-text includes/indexOf/regex checks in several assertions, which makes tests pass on wording-only changes and violates the repo’s no-source-grep testing policy. Please migrate these to parser-driven assertions (e.g., extractFencedCodeBlocks + shellStatements + command predicates) consistently.

Example direction (replace byte-offset text matching)
-const symbolicRefByteIdx = block.indexOf('symbolic-ref');
-const resetHardByteIdx = block.indexOf('reset --hard');
-assert.notStrictEqual(symbolicRefByteIdx, -1);
-assert.notStrictEqual(resetHardByteIdx, -1);
-assert.ok(symbolicRefByteIdx < resetHardByteIdx);
+const statements = shellStatements(block);
+const symbolicRefIdx = findCommandIndex(
+  statements,
+  (cmd) => cmd[0] === 'git' && cmd[1] === 'symbolic-ref' && cmd.includes('HEAD')
+);
+const resetHardIdx = findCommandIndex(
+  statements,
+  (cmd) => cmd[0] === 'git' && cmd[1] === 'reset' && cmd.includes('--hard')
+);
+assert.notStrictEqual(symbolicRefIdx, -1, 'symbolic-ref assertion must exist');
+assert.notStrictEqual(resetHardIdx, -1, 'reset --hard must exist');
+assert.ok(symbolicRefIdx < resetHardIdx, 'HEAD assertion must precede reset --hard');

Based on learnings: For this repository, follow the CONTRIBUTING.md “no-source-grep” testing standard and assert on parsed/structured representations instead of raw source-text matching.

Also applies to: 297-305, 308-313, 368-376, 435-454

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/bug-2924-worktree-head-attachment.test.cjs` around lines 202 - 210, The
test currently uses block.includes('update-ref'); replace this raw text check
with a parser-driven assertion: use extractFencedCodeBlocks (to locate the same
fenced/code block that populates the variable block in the test) and run
shellStatements on that block, then assert using a command predicate (e.g., a
hasCommand-like predicate that checks command === 'git' && subcommand ===
'update-ref') that the parsed statements do not contain a git update-ref
invocation (or, if the intent is to assert the prohibition exists as guidance,
assert there is a standalone comment/echo shell statement that explicitly
forbids update-ref). Update the test named "block forbids `git update-ref`
self-recovery in its guidance text" (and analogous checks at the other ranges)
to use extractFencedCodeBlocks + shellStatements + the command/comment predicate
instead of block.includes/indexOf/regex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/bug-2924-worktree-head-attachment.test.cjs`:
- Around line 255-258: The code slices subBlock using block.indexOf('**If
running as the sole', headingIdx) without checking for -1, which can cause an
incorrect slice; capture the end index into a variable (e.g., endIdx =
block.indexOf('**If running as the sole', headingIdx)), assert or throw if
endIdx === -1 (use assert.notStrictEqual or assert.ok with a clear message),
then use block.slice(headingIdx, endIdx) to build subBlock so the test fails
clearly when the subsection end marker is missing.

---

Nitpick comments:
In `@tests/bug-2924-worktree-head-attachment.test.cjs`:
- Around line 202-210: The test currently uses block.includes('update-ref');
replace this raw text check with a parser-driven assertion: use
extractFencedCodeBlocks (to locate the same fenced/code block that populates the
variable block in the test) and run shellStatements on that block, then assert
using a command predicate (e.g., a hasCommand-like predicate that checks command
=== 'git' && subcommand === 'update-ref') that the parsed statements do not
contain a git update-ref invocation (or, if the intent is to assert the
prohibition exists as guidance, assert there is a standalone comment/echo shell
statement that explicitly forbids update-ref). Update the test named "block
forbids `git update-ref` self-recovery in its guidance text" (and analogous
checks at the other ranges) to use extractFencedCodeBlocks + shellStatements +
the command/comment predicate instead of block.includes/indexOf/regex.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c1c3c550-6b7f-40e2-bea1-289696eee713

📥 Commits

Reviewing files that changed from the base of the PR and between ec57850 and 43c098a.

📒 Files selected for processing (7)
  • agents/gsd-executor.md
  • docs/CONFIGURATION.md
  • get-shit-done/bin/lib/config-schema.cjs
  • get-shit-done/workflows/execute-phase.md
  • get-shit-done/workflows/quick.md
  • sdk/src/query/config-schema.ts
  • tests/bug-2924-worktree-head-attachment.test.cjs
✅ Files skipped from review due to trivial changes (3)
  • get-shit-done/bin/lib/config-schema.cjs
  • sdk/src/query/config-schema.ts
  • docs/CONFIGURATION.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • agents/gsd-executor.md

Comment thread tests/bug-2924-worktree-head-attachment.test.cjs
@github-actions github-actions Bot added size/XL and removed size/XL labels May 1, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/bug-2924-worktree-head-attachment.test.cjs`:
- Around line 441-458: The test uses content.slice(idx, idx + 2000) which can
truncate the "Parallel agents" section; replace this fixed window with logic
that captures from idx to the next Markdown section boundary or end of content:
compute an end index by searching content (starting at idx) for the next heading
marker (e.g., a regex matching ^\s*#{1,6}\s in multiline mode) or other section
delimiter, and use content.slice(idx, endIdxOrContentEnd) to produce tail;
update the variables tail and sentences (and related assertions) accordingly so
the loop examines the entire "Parallel agents" section rather than a fixed
2,000-character substring.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4fecf907-4e2b-4b08-bde6-163c9e505ee4

📥 Commits

Reviewing files that changed from the base of the PR and between 43c098a and 03bbb5c.

📒 Files selected for processing (1)
  • tests/bug-2924-worktree-head-attachment.test.cjs

Comment thread tests/bug-2924-worktree-head-attachment.test.cjs
@github-actions github-actions Bot added size/XL and removed size/XL labels May 1, 2026
@trek-e trek-e merged commit 8de8ace into main May 1, 2026
14 checks passed
@github-actions github-actions Bot deleted the fix/2924-worktree-head-attachment branch May 1, 2026 13:23
github-actions Bot pushed a commit that referenced this pull request May 1, 2026
…ts (#2924) (#2941)

* fix(workflows): assert HEAD on per-agent branch before worktree commits

Worktree-mode setup could leave HEAD attached to a protected branch (master),
causing agent commits to land there. The previous response was a destructive
self-recovery via 'git update-ref refs/heads/master <sha>', which silently
rewinds the protected branch and destroys concurrent commits in multi-active
scenarios (parallel agents, user committing while agent runs).

- Reorder <worktree_branch_check> in execute-phase.md and quick.md to assert
  HEAD via 'git symbolic-ref' BEFORE any 'git reset --hard'. HALT with a
  blocker if HEAD is on main/master/develop/trunk/release/* or detached.
- Add a per-commit HEAD assertion (step 0) to gsd-executor.md
  <task_commit_protocol>; HEAD attachment can drift after 'git checkout <sha>'.
- Forbid 'git update-ref refs/heads/<protected>' in
  <destructive_git_prohibition>; surface the blocker rather than self-heal.
- Remove '--no-verify' as the worktree-mode default in execute-phase.md,
  execute-plan.md, quick.md, and references/git-integration.md. Hooks now
  run on every executor commit; opt out only via workflow.worktree_skip_hooks.
- Add regression test that parses the worktree_branch_check blocks structurally
  and asserts the symbolic-ref check precedes the reset --hard, no workflow
  performs update-ref on a protected ref, and --no-verify is no longer the
  default in any parallel-execution prompt.

* fix(#2924): address CodeRabbit review findings on worktree HEAD PR

- Add positive worktree-agent-* allow-list to <task_commit_protocol> step 0
  in gsd-executor.md and to <worktree_branch_check> in execute-phase.md and
  quick.md. The deny-list (main|master|develop|trunk|release/*) silently
  allowed feature/* and other arbitrary branches outside the agent namespace.
- Register workflow.worktree_skip_hooks in both config schemas
  (sdk/src/query/config-schema.ts and get-shit-done/bin/lib/config-schema.cjs)
  and document it in docs/CONFIGURATION.md so config-set accepts it.
- Fix stash lifecycle in execute-phase.md post-wave hook validation: stash
  under a named ref and pop after the hook run; warn on pop failure.
- Pre-dispatch PLAN.md commit in quick.md: gate on git diff --cached --quiet
  for idempotency and exit 1 with a clear error on commit failure (both the
  --no-verify and the normal branches) — no more swallowing real errors.
- Test fixes (tests/bug-2924-worktree-head-attachment.test.cjs):
  - Parse the protected-branch alternation structurally and require
    main, master, develop, trunk, release/.* (release/* was previously
    skipped by the \\b...\\b regex).
  - Use fs.readdirSync(dir, { recursive: true }) so workflows in nested
    subdirectories are also asserted against the update-ref ban.
  - Add allow-list assertions for execute-phase.md, quick.md, and
    gsd-executor.md to lock in the new positive namespace check.

* test(#2924): assert sub-section end marker exists before slicing

* test(#2924): use section boundary instead of fixed window for parallel-agents slice

(cherry picked from commit 8de8ace)
trek-e added a commit that referenced this pull request May 1, 2026
…d section (#2991)

Both v1.39.0 (stable, tagged 2026-05-01T03:05:33Z) and v1.39.1
(hotfix, tagged 2026-05-01T21:03:54Z) shipped to npm but the
CHANGELOG `[Unreleased]` link still pointed at `v1.38.5...HEAD` and
the entries that landed in v1.39.1 were still un-promoted.

Move the five v1.39.1 hotfix entries (#2917, #2949, #2954, #2962,
#2969) into a new `## [1.39.1] - 2026-05-01` section above
`## [1.38.5]`, with a one-line intro and install snippet matching
the conventions used in earlier dated sections.

Update the `[Unreleased]` link to point at `v1.39.1...HEAD`.

Out of scope (separate cleanup):
  - Backfilling a `## [1.39.0]` section. The CHANGELOG never had one;
    this PR doesn't make that worse but also doesn't try to invent
    release-note text from commit messages.
  - The eight v1.39.1 commits without `[Unreleased]` entries
    (#2942, #2944, #2924/#2941, #2940, #2947, #2950, #2948, #2957).
    These weren't in `[Unreleased]` to begin with; faithful
    promotion only moves what was already documented.
  - Adding a `docs/RELEASE-v1.39.1.md` file. The `docs/RELEASE-*.md`
    pattern in this repo is RC-only; stable patches historically
    don't have a counterpart.

The post-v1.39.1 hardening entries (#2980, #2983, #2987 from this
session, plus #2976 which was pre-skipped from the v1.39.1
cherry-pick set after #2980 landed) remain in the new
`[Unreleased]` section — they ship in the next release.

Closes #2989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(execute-plan): worktree HEAD attaches to master; agent commits land on master; "recovery" force-rewinds master via git update-ref

1 participant